-
-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v0.2 wip #72
Conversation
*/ | ||
const edit = async (file, contents) => { | ||
const updatedPromise = new Promise((resolve) => { | ||
// FIXME? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix what? that the code below doesn't include the filename in the resolve check anymore? that could lead to false positives if there are multiple edits in parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's it.
As a matter of fact, this doesn't affect currently existing tests and, even though I don't remember, there was a reason I removed it.
My take is go with this version until we actually have the need because of some new tests... Which might very well be never, since focus will now shift on Svelte 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't check all the tests for soundness and the syntax/DSL for defining tests takes a bit to get used to but +1 to it being less verbose than classic playwright tests.
github matrix and package.json look ok.
needs some changeset files to describe the changes done to the package (dropped support for nollup, node12 etcpp)
Co-authored-by: Ben McCann <[email protected]>
4ac617a
to
22769e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!!
eslint 9 is out now and is another potential upgrade that could be done, but I wouldn't block on it. it's also possible that some of the pnpm overrides are no longer necessary with the latest versions of all packages
Indeed... I've removed them.
I gave it a shot, but it's not playing well with pnpm and the current config so, yeah, I'm leaving out of this MR. |
No description provided.